This feels like pain. When you have code, you can pass a map, when you are at the command line, it will make the input unreadable. I really fail to see how somenoe will pass Java properties format via a single CLI.

AI Review — PR #63: [MRESOLVER-126] support conversion from String to Map

Hi @kwin, thanks for working on this — the ability to set map-valued config properties from the CLI addresses a real usability gap!

However, this PR is now 5+ years old (opened June 2020) and has accumulated several issues:

🔴 Merge conflicts (high)

The PR is in a CONFLICTING merge state. One of the three target files (src/site/markdown/configuration.md) has been completely removed from master. ConfigUtils.java has also been substantially reformatted and refactored since 2020.

🔴 No CI verification (high)

No CI checks have ever run on this branch. There's no automated verification that the code compiles or passes tests against current master.

⚠️ Unresolved design concern from maintainer (medium)

@michael-o expressed skepticism about parsing Java Properties format from a CLI system property: "This feels like pain... I really fail to see how someone will pass Java properties format via a single CLI." The discussion ended without consensus, with michael-o suggesting the issue be put aside "until someone really complains."

⚠️ Silent exception swallowing (medium)

The catch (IOException e) { // try next key } silently swallows the exception without logging. While StringReader is unlikely to throw IOException in practice, at minimum a TRACE/DEBUG log would aid maintainability.

💡 Properties vs HashMap return type (low)

The method returns a Properties object (which extends Hashtable) from a method declared to return Map<?, ?>. Callers get a synchronized Hashtable-backed instance rather than a plain HashMap. Converting to new HashMap<>(props) before returning would be more consistent.

Recommendation: Given the age, merge conflicts, missing target file, and unresolved design discussion, this PR would need a fresh start. If the feature is still desired, a new PR rebased on current master with a CLI-friendlier format (e.g., key=value,key2=value2) might address the maintainer's concerns better.

🤖 This review was generated by ForgeBot using a maker/checker pattern (reviewer + independent verifier). 1 of 6 original findings was removed as a false positive after verification (project compiles at Java 8, not 21+).

This PR was opened over 6 years ago (June 2020) and is in a CONFLICTING merge state. The feature idea (String-to-Map conversion for CLI use) has merit, but the implementation needs a complete rewrite against current master.

Structural incompatibility (blockers):

• src/site/markdown/configuration.md was removed from the repository (commit 08f8381d by the PR author themselves). The documentation change targets a file that no longer exists.
• The getMap(RepositorySystemSession, ...) overload was refactored — it now delegates to getMap(Map<?,?>, ...) at line 321. The PR patches the old inline logic which no longer exists at the targeted location.

Code quality observations:

• The catch (IOException e) { // try next key } block silently swallows exceptions without logging.
• The method returns a java.util.Properties instance (extends Hashtable, synchronized) — returning new HashMap<>(props) would be cleaner for a Map<?,?> return type.
• Code formatting uses old-style conventions (spaces before parentheses, braces on separate lines) that don't match current project style.
• Test uses public void visibility while current conventions use package-private void.

Unresolved design concern:
Maintainer @michael-o raised a concern in 2020 about the usability of Java Properties format via CLI: "This feels like pain... I really fail to see how someone will pass Java properties format via a single CLI." This was never addressed. A simpler format like key=value,key2=value2 might be more CLI-friendly.

Recommendation: If this feature is still desired, a fresh PR rebased on current master would be needed. The String conversion logic should go into the getMap(Map<?,?>, Map<?,?>, String...) overload at line 321.

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of gnodet